fix(cypher): frontend should always emit exclusive kind matchers BED-7495#37
Open
fix(cypher): frontend should always emit exclusive kind matchers BED-7495#37
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an IsExclusive boolean to KindMatcher, sets it when parsing node labels, carries it through constructors/copies, adjusts PostgreSQL kind-ID translation to pick array-contains vs overlap based on IsExclusive, and migrates kind-mapper state to instance-level maps and methods. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Frontend as Frontend Parser
participant Model as KindMatcher Model
participant Translator as PG Translator
participant Postgres as PostgreSQL
Client->>Frontend: submit query with node labels
Frontend->>Model: NewKindMatcher(..., isExclusive=true)
Model->>Translator: translateKindMatcher(kindMatcher{IsExclusive})
Translator->>Translator: select operator based on IsExclusive (array-contains vs overlap)
Translator->>Postgres: emit SQL using chosen operator
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
query/model.go (1)
132-137:Kind()relies on zero-value forIsExclusivewhile sibling functions are explicit.
DeleteKind,DeleteKinds, andKindInall explicitly setIsExclusive: false, butKind()creates aKindMatchervia struct literal without it. The zero-value behavior is correct, but adding the explicit field (or using the constructor) would be consistent with the rest of this PR.♻️ Consistency suggestion
func Kind(reference graph.Criteria, kinds ...graph.Kind) *cypherModel.KindMatcher { - return &cypherModel.KindMatcher{ - Reference: reference, - Kinds: kinds, - } + return cypherModel.NewKindMatcher(reference, kinds, false) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@query/model.go` around lines 132 - 137, Kind() currently relies on the zero-value for KindMatcher.IsExclusive while sibling functions (DeleteKind, DeleteKinds, KindIn) set IsExclusive: false explicitly; update Kind() to set IsExclusive: false in the returned cypherModel.KindMatcher struct literal (or call the same constructor used by the siblings) so it matches the explicit behavior of DeleteKind/DeleteKinds/KindIn and keeps consistency across the API.cypher/models/pgsql/translate/kind.go (1)
12-34:isExclusiveis silently ignored for edge/composite bindings.For edges (line 26–30), the function always emits
= ANY(...)regardless ofisExclusive. This is likely correct since edges carry a singlekind_id, but it's worth a brief comment at the edge branch to make this explicit — prevents future maintainers from wondering if it's an oversight.📝 Suggested comment
case pgsql.EdgeComposite, pgsql.ExpansionEdge: + // Edges have a single kind_id, so the exclusive/overlap distinction does not apply. treeTranslator.PushOperand(pgsql.CompoundIdentifier{binding.Identifier, pgsql.ColumnKindID})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/models/pgsql/translate/kind.go` around lines 12 - 34, The branch in newPGKindIDMatcher that handles binding.DataType == pgsql.EdgeComposite or pgsql.ExpansionEdge ignores the isExclusive flag (it always emits an = ANY(...) expression); add a concise comment above the edge branch explaining that edges store a single kind_id so exclusivity is not applicable and isExclusive is intentionally ignored for EdgeComposite/ExpansionEdge, referencing newPGKindIDMatcher, binding.DataType, and the edge branch to make intent explicit for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cypher/models/pgsql/translate/kind.go`:
- Around line 12-34: The branch in newPGKindIDMatcher that handles
binding.DataType == pgsql.EdgeComposite or pgsql.ExpansionEdge ignores the
isExclusive flag (it always emits an = ANY(...) expression); add a concise
comment above the edge branch explaining that edges store a single kind_id so
exclusivity is not applicable and isExclusive is intentionally ignored for
EdgeComposite/ExpansionEdge, referencing newPGKindIDMatcher, binding.DataType,
and the edge branch to make intent explicit for future maintainers.
In `@query/model.go`:
- Around line 132-137: Kind() currently relies on the zero-value for
KindMatcher.IsExclusive while sibling functions (DeleteKind, DeleteKinds,
KindIn) set IsExclusive: false explicitly; update Kind() to set IsExclusive:
false in the returned cypherModel.KindMatcher struct literal (or call the same
constructor used by the siblings) so it matches the explicit behavior of
DeleteKind/DeleteKinds/KindIn and keeps consistency across the API.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cypher/frontend/expression.gocypher/models/cypher/model.gocypher/models/pgsql/translate/kind.goquery/model.go
seanjSO
force-pushed
the
seanj/BED-7495
branch
2 times, most recently
from
ad5ef1e to
7a63ae4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cypher/models/pgsql/translate/kind.go`:
- Around line 12-24: Add a guard in newPGKindIDMatcher to handle empty kindIDs
when isExclusive is true: if isExclusive && len(kindIDs) == 0, push a boolean
FALSE literal (e.g. pgsql.NewLiteral(false, pgsql.Boolean)) into treeTranslator
via treeTranslator.PushOperand and return as a completed expression so the
matcher yields no rows; otherwise keep the existing logic that uses
pgsql.OperatorPGArrayLHSContainsRHS and pgsql.OperatorPGArrayOverlap. This
ensures the @> path (OperatorPGArrayLHSContainsRHS) does not vacuously match all
rows when kindIDs is empty.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cypher/frontend/expression.gocypher/models/cypher/model.gocypher/models/pgsql/translate/kind.goquery/model.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cypher/models/cypher/model.go
- cypher/frontend/expression.go
seanjSO
force-pushed
the
seanj/BED-7495
branch
from
7a63ae4 to
7dadc28
Compare
cweidenkeller
approved these changes
Feb 26, 2026
|
approved with the stipulation of some scenario tests =) |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/pg/pgutil/kindmapper.go (1)
89-95:⚠️ Potential issue | 🟠 MajorMake
Putidempotent for existing kinds.Line 90 always allocates a fresh ID. If
Putis called twice for the same kind, the mapper ends up with multiple IDs for one kind (stale reverse entries), which breaks one-to-one mapping guarantees.🔧 Proposed fix
func (s *InMemoryKindMapper) Put(kind graph.Kind) int16 { + if existingID, found := s.KindToID[kind]; found { + return existingID + } + kindID := s.nextKindID s.nextKindID += 1 s.KindToID[kind] = kindID s.IDToKind[kindID] = kind return kindID }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drivers/pg/pgutil/kindmapper.go` around lines 89 - 95, The Put method on InMemoryKindMapper currently always assigns a fresh ID (using nextKindID) causing duplicate mappings; update Put to first check s.KindToID for an existing entry and return that existing int16 if present, otherwise allocate a new ID, set both s.KindToID[kind]=id and s.IDToKind[id]=kind and increment s.nextKindID, ensuring no stale reverse entries are left and preserving one-to-one mapping between KindToID and IDToKind.
🧹 Nitpick comments (1)
drivers/pg/pgutil/kindmapper.go (1)
12-13: Avoid exposing mutable mapping internals directly.Lines 12-13 expose both maps publicly, so callers can mutate one side and break
KindToID/IDToKindconsistency. Prefer private fields with read-only accessors (or snapshot getters) to preserve invariants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drivers/pg/pgutil/kindmapper.go` around lines 12 - 13, Change the exported map fields KindToID and IDToKind to unexported fields (e.g., kindToID, idToKind) to prevent external mutation, update the constructor/factory that populates them, and provide read-only accessors such as GetID(kind graph.Kind) (int16, bool) and GetKind(id int16) (graph.Kind, bool); if callers need the whole mapping, add snapshot methods like KindToIDSnapshot() map[graph.Kind]int16 and IDToKindSnapshot() map[int16]graph.Kind that return copies to preserve internal invariants and keep the two maps consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@drivers/pg/pgutil/kindmapper.go`:
- Around line 89-95: The Put method on InMemoryKindMapper currently always
assigns a fresh ID (using nextKindID) causing duplicate mappings; update Put to
first check s.KindToID for an existing entry and return that existing int16 if
present, otherwise allocate a new ID, set both s.KindToID[kind]=id and
s.IDToKind[id]=kind and increment s.nextKindID, ensuring no stale reverse
entries are left and preserving one-to-one mapping between KindToID and
IDToKind.
---
Nitpick comments:
In `@drivers/pg/pgutil/kindmapper.go`:
- Around line 12-13: Change the exported map fields KindToID and IDToKind to
unexported fields (e.g., kindToID, idToKind) to prevent external mutation,
update the constructor/factory that populates them, and provide read-only
accessors such as GetID(kind graph.Kind) (int16, bool) and GetKind(id int16)
(graph.Kind, bool); if callers need the whole mapping, add snapshot methods like
KindToIDSnapshot() map[graph.Kind]int16 and IDToKindSnapshot()
map[int16]graph.Kind that return copies to preserve internal invariants and keep
the two maps consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor